Skip to content

Conversation

@xjerwa
Copy link
Contributor

@xjerwa xjerwa commented Dec 3, 2018

No description provided.

@xjerwa xjerwa requested a review from bengry December 3, 2018 22:44
interface EventListener<K extends keyof ElementEventMap> {
type: K;
listener: (ev: ElementEventMap[K]) => void;
interface EventListener {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this update be all right? I tested it locally and didn't notice anything broken.

This was to fix the issue I was seeing since EventListener is a default type that is not generic:
All declarations of 'EventListener' must have identical type parameters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just changing the name of the interface?
I missed the fact that it was already taken (looking back, should really have checked it + assume such a generic name would be taken).

even changing to something like IEventListener would solve this. If you have suggestions for a better name - I'm open for suggestions (thought of EventListenerOptions, but it's taken - see line 4 here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think IEventListener is fitting. I've updated the name and added the generic typing back. Are there any references to IEventListener in other places in the code that I should also be updating?

Copy link
Contributor

@bengry bengry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments, looking good overall - just the fabric upgrade requires attention, and a few other minor things I left comments on that would be nice to have, but not critical.

@bengry bengry merged commit bc39370 into master Dec 4, 2018
@bengry bengry deleted the pr/45 branch December 4, 2018 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants